fix(pmm-fb): follow PMM's secrets directory to /srv/sep - #1382
Merged
Conversation
PMM now writes SEP's secrets to /srv/sep from a volume named pmm-sep, consolidated so a second thing it hands SEP lands beside the first rather than in a directory of its own. Mounting the old names gets an empty volume and no secrets, which SEP reports only as unset settings. The Grafana service-account token is no longer among them: PMM will not provision it, so its two files and the two-stage write they caused are gone from the harness docs.
6 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the sidecar/pmm-fb harness to follow PMM’s consolidated secrets volume/path change (volume renamed to pmm-sep, PMM mount path now /srv/sep) while keeping the side-car’s SECRETS_DIR mount (/run/secrets/sep) unchanged.
Changes:
- Rename the shared secrets volume reference from
pmm-sep-secrets→pmm-sepand update the PMM-side mount path/srv/sep-secrets→/srv/sep. - Update harness documentation to reflect the reduced secrets file set (Grafana token channel removed) and the updated “wait for health” rationale.
- Refresh bootstrap header comments to match the new volume naming.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sidecar/pmm-fb/README.md | Updates docs to match the consolidated secrets volume/path and reduced file set. |
| sidecar/pmm-fb/compose.yaml | Renames the shared volume and updates mount points for pmm-server and sep-sidecar. |
| sidecar/pmm-fb/bootstrap.sh | Updates header comment to reference the new secrets volume name. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - pmm-data:/srv | ||
| # The only volume crossing the boundary; pmm-data stays PMM's alone | ||
| - pmm-sep-secrets:/srv/sep-secrets | ||
| - pmm-sep:/srv/sep |
8 tasks
yyyyyyyan
added a commit
that referenced
this pull request
Aug 21, 2026
…ts start period PMM_FB_TAG moves from PR-4500-882b6ba to PR-4500-73298a4, in both places the default is spelled. 882b6ba was derived from a tree still containing PMM-15280 and PMM-15331, both of which were closed unmerged on 2026-08-20, and it could not bring the pair up at all: pmm-server exited on the PMM_SEP_POSTGRES_PASSWORD that #1383 stopped seeding, and then on /srv/sep not being writable, because that build wrote /srv/sep-secrets and did not ship the directory #1382 mounts. 73298a4 fixes all three. Measured on the artifact: /srv/sep ships 2770 uid 1000 gid 0 and seeds into the mounted volume; the secrets directory holds four files and no Grafana token; both containers reach healthy; an authenticated /sep/api/apps/ round trip answers 200. Losing PMM-15331 also lost the widened start period it carried. The image ships 25s with 3 retries at 4s, so pmm-server is marked unhealthy around 37s, while a cold start here takes ~72s to first pass readyz — and because sep-sidecar depends on service_healthy, compose aborted the dependent rather than waiting. Observed on both green tags before this change. The harness now overrides the healthcheck with a 300s start period, which is the property the build used to provide; `docker compose up -d` completes in one invocation again. sep-sidecar keeps condition: service_healthy. The README's standing advice was to fall back to service_started when repinning to a build without the gate, but that trades an aborted bring-up for a side-car that wins the race and exits on a missing SECRET_KEY. Health still means the four files exist; the start period is what gives a cold start room to get there. Two doc claims the repin invalidates are corrected with it: the caveat citing PMM-15331 as the source of the start period, and the count of files removed when PMM_ENABLE_SEP is unset, which is four rather than six now that the two Grafana token names are gone.
yyyyyyyan
added a commit
that referenced
this pull request
Aug 24, 2026
… start period (#1396) ## Summary The side-car now obtains its own Grafana service-account token at container start (landed on `main` as part of SEP-1871). Three harness changes follow. **Side-car repin.** `sep-sidecar` pinned `…:29d29624`, a main-line tag predating the mint. Repinned to `…:95a70382`, the main-line commit that added it — currently both `main`'s tip and the newest published tag. Verified on the published artifact rather than on the commit, in the style the harness README's "Which image to pin" section already prescribes: | Property | Observed | |---|---| | helper present | `grafana_service_account.py` in the app root | | state directory | the side-car's state dir, mode `700`, owned `1001:1001` | | fan-out function | `export_grafana_token` in `settings-env.sh` | | reads secrets from a directory | 4 `SECRETS_DIR` references in `settings-env.sh` | | carries a HEALTHCHECK | `CMD healthcheck.sh`, start period 150s | **`sep-state` volume.** The side-car persists the minted token mode `0600` under its state directory and re-reads it on the next start, so a restart needs no Grafana admin credential. The volume extends that across a container *recreate*. Without it every recreate mints a further token on the same service account, and since minted tokens are asked not to expire, the ones earlier containers resolved stay valid in Grafana until an operator deletes them. **pmm-server repin, and owning its start period.** `PMM_FB_TAG` moves from `PR-4500-882b6ba` to `PR-4500-73298a4`, in both places the default is spelled. That tag was derived from a tree still containing PMM-15280 and PMM-15331, both closed unmerged on 2026-08-20, and could not bring the pair up at all — pmm-server exited on the `PMM_SEP_POSTGRES_PASSWORD` that #1383 stopped seeding, then on `/srv/sep` not being writable, since that build wrote `/srv/sep-secrets` and did not ship the directory #1382 mounts. Losing PMM-15331 also lost the widened start period it carried. The image ships 25 s with 3 retries at 4 s, so pmm-server is marked `unhealthy` around 37 s while a cold start here takes ~72 s to first pass `readyz` — and because `sep-sidecar` depends on `service_healthy`, compose aborted the dependent rather than waiting. This was reproducible on *both* green feature builds. `compose.yaml` now sets a 300 s start period of its own, restoring what the build used to provide, and `docker compose up -d` completes in one invocation again. That start period is the only healthcheck field it sets: Docker merges the healthcheck field by field, so the probe and the remaining timings keep tracking whatever the pinned image ships rather than a copy that goes stale across repins. `sep-sidecar` keeps `condition: service_healthy`. The README's standing advice was to fall back to `service_started` when repinning to a build without the gate; that trades an aborted bring-up for a side-car that wins the race and exits on a missing `SECRET_KEY`. Health still means the four secret files exist — the start period is what gives a cold start room to reach that point. The doc claims the repin invalidates are corrected alongside it: the caveat citing PMM-15331 as the source of the start period, the count of files removed when `PMM_ENABLE_SEP` is unset, which is four now that the two Grafana token names are gone, and four statements that PMM generates or owns *every* SEP secret — true while PMM published the Grafana token, and now false by exactly that one credential. Those are scoped to the four files SEP reads from disk, and the token gets its own bullet recording that the side-car owns it while a build publishing the two canonical names still outranks the mint. `compose.yaml`'s own header carried two of them, alongside a `Run ./bootstrap.sh first` line the degating below it had just retired. The repin checklist gains the property this branch makes load-bearing. "Which image to pin" listed two things a pin must carry; the `sep-state` volume adds a third, because an image without the mint passes both existing checks, comes up healthy, and mounts the volume over a directory nothing writes to — with inert Grafana auth as the only symptom, and nothing logged. The old pin this PR replaces was exactly such an image, so the checklist meant to catch it now probes for the mint helper and the state directory. The bring-up section's token paragraph, which credited PMM with minting and publishing the token, now describes the side-car's own mint, the volume that persists it, and the fact that a token arriving through `SECRETS_DIR` still outranks it. **`bootstrap.sh` is now genuinely mysql-only.** It generates three `sep-mysql` test-fixture passwords and nothing else — PMM provisions every secret the pair needs — but `compose.yaml` guarded them with `${VAR:?run ./bootstrap.sh first}`, and Compose interpolates every service at parse time regardless of active profiles. So a guard on this profile-gated service made the script a prerequisite of the two-service bring-up too: with the generated dotenv absent, `docker compose config` exited 1 on *both* profiles. The three are now passed through unset. This removes a duplicated guard rather than weakening one — `mysql-entrypoint.sh` defines `require_secrets` with the same three names and the same message, and calls it unconditionally at top level before any use, so a mysql run without `bootstrap.sh` still fails loudly, at container start instead of at parse. ## Tested Measured on this branch against `PR-4500-73298a4`, and separately against `PR-4500-7dcd575`, with no overrides beyond a host port remap (an unrelated pmm-server holds 8443 on this machine). Torn down with `down -v` after each run. - With **the generated dotenv absent entirely**, `docker compose config` exits 0 on both profiles, and `docker compose up -d` reaches `sep-sidecar` healthy with four secret files, a token minted mode `600` owned `1001:1001`, and one `sep` service account in Grafana. - `docker compose up -d` completes in one invocation, ~72 s, both containers healthy. Without the start-period override the same command aborts with `dependency failed to start: container … is unhealthy`. - The side-car's secrets directory holds exactly four files — the three database passwords and `SECRET_KEY` — and no Grafana token, which is the "no token configured anywhere" precondition. - Grafana shows exactly one service account: `id=2 name=sep role=Admin tokens=1`. - The minted token is persisted mode `600` owned `1001:1001` in the `sep-state` volume. - All five supervised programs — `sep`, `inventory`, `tasks`, `celery-worker`, `celery-beat` — carry both canonical names, each equal to the persisted token, read from each process's own `/proc` environ. - Grafana-backed sign-in end to end through PMM's nginx: `/graph/login` 200 → session exchange → SEP bearer → `/sep/api/apps/` 200 with four apps. - The PMM-facing path round-trips: from inside `celery-worker`'s own environment, `PMM.api_key` resolves and an authenticated `GET /graph/api/org/users` returns one org user. That is the call that previously no-opped silently. - Restarting the side-car reuses the persisted token byte-for-byte; Grafana still reports one account with one token. - Neither canonical token name nor `GF_SECURITY_ADMIN_*` appears in `docker inspect` output. Left for QA in its own environment: - [ ] Recreate the side-car (`docker compose up -d --force-recreate sep-sidecar`) and confirm the token survives, which is what the `sep-state` volume buys over a plain restart. - [ ] Confirm a PMM build that *does* publish the two Grafana token names into the secrets directory still outranks the mint — the side-car should make no Grafana call and use the published token. - [ ] Bring up with `--profile mysql` and confirm the `sep-mysql` build picks up the same `PMM_FB_TAG` default. Note for anyone repeating this: a `docker exec` into the side-car starts from the image's declared environment and never sees PID 1's exports, so probing the token that way reports it unset even when every supervised program has it. That is the design working — inheritance is also why it stays out of `docker inspect` — but it reads like a failure. Read a supervised child's `/proc/<pid>/environ` instead. ## Known limitations - Feature-build tags are a moving target: PMM-15216, PMM-15293 and PMM-15294 are still open and their branches get rewritten without notice. If a bring-up starts failing with nothing apparently changed on either side, suspect a silent re-derivation of `PMM-15205-sep-fb` before looking at SEP. ## Checklist - [x] ~~New/modified functions have type hints and rST docstrings~~ *(N/A — compose and docs only)* - [x] ~~New tests added for new features or bug fixes~~ *(N/A — the mint's tests live on `main`)* - [x] ~~Database migrations generated if models changed~~ *(N/A for this change)* - [x] User-facing changes documented (README, inline help, UI text) - [x] Configuration changes documented with examples
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follows PMM's rename of the shared secrets directory. The harness currently mounts names PMM no longer writes to, so without this the side-car gets an empty volume and comes up with every file-supplied setting unset.
pmm-sep-secretspmm-sep/srv/sep-secrets/srv/sep/run/secrets/sepSECRETS_DIR/run/secrets/sepThe directory was consolidated on the PMM side (percona/pmm#5762) so that a second credential PMM hands SEP lands beside the first rather than in a directory of its own — the alternative being
/srv/sep-secrets,/srv/sep-something-else, and so on. The volume follows the path it is mounted at.The Grafana service-account token is no longer part of this channel. percona/pmm#5755 is closed: PMM will not provision the account or publish the token, so
AUTH__PROVIDER__GRAFANA__SERVICE_ACCOUNT_TOKENandPMM__API_KEYare gone from the table, the file count drops from six to four, and the two-stage write those files caused no longer happens — the entrypoint now writes all four.Known gap this does not close. With that channel gone and nothing replacing it yet, the harness has no Grafana token at all, so Grafana-backed sign-in and the PMM syncer stay inert. The agreed replacement is for SEP to mint its own token against
POST /api/serviceaccounts, authenticating withGF_SECURITY_ADMIN_USER/GF_SECURITY_ADMIN_PASSWORDand falling back to theadmin/admindefault, retrying until Grafana answers, and storing the result. That is separate work and not in this PR.Worth recording for it: the admin credential is needed whenever SEP holds no valid token, which is not only first boot — a Grafana volume wipe or a restore predating the service account kills the stored token, and re-minting then fails if the admin password was changed through
change-admin-passwordrather than the environment. SettingGF_SECURITY_ADMIN_PASSWORDand giving SEP the same value avoids it.Testing
compose.yamlparses and both services resolve the renamed volume (pmm-server: pmm-sep:/srv/sep,sep-sidecar: pmm-sep:/run/secrets/sep:ro).shellcheckclean onbootstrap.sh. No reference to the old names survives undersidecar/pmm-fb/.End-to-end bring-up is not exercised here: it needs a PMM image built from percona/pmm#5762, which is not merged.